-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a convenience object for expressing once-like / per-runtime patterns #55793
base: master
Are you sure you want to change the base?
Conversation
base/exports.jl
Outdated
PerProcess, | ||
PerTask, | ||
PerThread, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bikeshedding in a thread..
Something seemed off about Per*
on first impression. I think I was expecting there to be a n
somewhere i.e. not specifically tied to once.
What about ProcessSingleton / ThreadSingleton / TaskSingleton ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Java used *Local naming convention for this construct, so in this context, that would be: ProcessLocal, ThreadLocal, TaskLocal
:-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the condext of a do
block, I think OncePerProcess
is great.
"Once per process do function body" if a fairly accurate description of what this does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To help paint the bikeshed, I've come across a word that I think bears mentioning: "Hapax". It's probably a bit to esoteric to seriously consider, but it apparently refers to something (usually a word or expression) that occurs only once within a given context.
That seems like a pretty excellent semantic fit to me 😀
Other than that, OncePerProcess
, ProcessSingleton
, or ProcessInitializer
seem like solid options to me.
PerProcess: once per process PerThread: once per thread id PerTask: once per task object
Somewhat generalizes our support for changing Ptr to C_NULL. Not particularly fast, since it is just using the builtins implementation of setfield, and delaying the actual stores, but it should suffice.
Prior to this, especially on macOS, the gc-safepoint here would cause the process to segfault as we had already freed the current_task state. Rearrange this code so that the GC interactions (except for the atomic store to current_task) are all handled before entering GC safe, and then signaling the thread is deleted (via setting current_task = NULL, published by jl_unlock_profile_wr to other threads) is last. ``` ERROR: Exception handler triggered on unmanaged thread. Process 53827 stopped * thread #5, stop reason = EXC_BAD_ACCESS (code=2, address=0x100018008) frame #0: 0x0000000100b74344 libjulia-internal.1.12.0.dylib`jl_delete_thread [inlined] jl_gc_state_set(ptls=0x000000011f8b3200, state='\x02', old_state=<unavailable>) at julia_threads.h:272:9 [opt] 269 assert(old_state != JL_GC_CONCURRENT_COLLECTOR_THREAD); 270 jl_atomic_store_release(&ptls->gc_state, state); 271 if (state == JL_GC_STATE_UNSAFE || old_state == JL_GC_STATE_UNSAFE) -> 272 jl_gc_safepoint_(ptls); 273 return old_state; 274 } 275 STATIC_INLINE int8_t jl_gc_state_save_and_set(jl_ptls_t ptls, Target 0: (julia) stopped. (lldb) up frame #1: 0x0000000100b74320 libjulia-internal.1.12.0.dylib`jl_delete_thread [inlined] jl_gc_state_save_and_set(ptls=0x000000011f8b3200, state='\x02') at julia_threads.h:278:12 [opt] 275 STATIC_INLINE int8_t jl_gc_state_save_and_set(jl_ptls_t ptls, 276 int8_t state) 277 { -> 278 return jl_gc_state_set(ptls, state, jl_atomic_load_relaxed(&ptls->gc_state)); 279 } 280 #ifdef __clang_gcanalyzer__ 281 // these might not be a safepoint (if they are no-op safe=>safe transitions), but we have to assume it could be (statically) (lldb) frame #2: 0x0000000100b7431c libjulia-internal.1.12.0.dylib`jl_delete_thread(value=0x000000011f8b3200) at threading.c:537:11 [opt] 534 ptls->root_task = NULL; 535 jl_free_thread_gc_state(ptls); 536 // then park in safe-region -> 537 (void)jl_gc_safe_enter(ptls); 538 } ```
Address reviewer feedback, add more fixes and more tests
@@ -500,7 +507,7 @@ Create a level-triggered event source. Tasks that call [`wait`](@ref) on an | |||
After `notify` is called, the `Event` remains in a signaled state and | |||
tasks will no longer block when waiting for it, until `reset` is called. | |||
|
|||
If `autoreset` is true, at most one task will be released from `wait` for | |||
If `autoreset` is true, at most one task will be released from `wait` for) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mistake?
@@ -500,7 +507,7 @@ Create a level-triggered event source. Tasks that call [`wait`](@ref) on an | |||
After `notify` is called, the `Event` remains in a signaled state and | |||
tasks will no longer block when waiting for it, until `reset` is called. | |||
|
|||
If `autoreset` is true, at most one task will be released from `wait` for | |||
If `autoreset` is true, at most one task will be released from `wait` for) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If `autoreset` is true, at most one task will be released from `wait` for) | |
If `autoreset` is true, at most one task will be released from `wait` for |
@@ -70,6 +70,9 @@ export | |||
OrdinalRange, | |||
Pair, | |||
PartialQuickSort, | |||
OncePerProcess, | |||
OncePerTask, | |||
OncePerThread, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably not be exported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*This = OncePerThread only. And still public, just not exported.
Warning: it is not necessarily true that a Task only runs on one thread, therefore the value | ||
returned here may alias other values or change in the middle of your program. This type may | ||
get deprecated in the future. If initializer yields, the thread running the current task | ||
after the call might not be the same as the one at the start of the call. | ||
|
||
See also: [`OncePerTask`](@ref). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps lead with this warning and the admonition to almost always use OncePerTask instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably also be a !!! warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type may get deprecated in the future.
Should it be under the Experimental
module namespace?
end | ||
return xs[tid] | ||
end | ||
@inline (once::OncePerThread)() = once[Threads.threadid()] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not once(threadid)
?
function once::OncePerThread(threadid=Threads.threadid())
...
end
julia> const global_state = Base.OncePerProcess{Vector{UInt32}}() do | ||
println("Making lazy global value...done.") | ||
return [Libc.rand()] | ||
end; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
end; | |
end | |
... |
Showing the return type here may be helpful. That will show us that global_state isa Base.OncePerProcess
.
``` | ||
""" | ||
mutable struct OncePerProcess{T, F} | ||
x::Union{Nothing,T} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
x::Union{Nothing,T} | |
value::Union{Nothing,T} |
julia> procstate = global_state(); | ||
Making lazy global value...done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
julia> procstate = global_state(); | |
Making lazy global value...done. | |
julia> procstate = global_state() | |
Making lazy global value...done. | |
[0x97b92da8] |
Everyone at triage was happy with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments from triage.
const PerStateConcurrent = 0x03 | ||
|
||
""" | ||
OncePerProcess{T} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OncePerProcess{T} | |
OncePerProcess{T}(init::Function) | |
OncePerProcess{T}(init::Function)() -> T |
This seems more suggestive of how this is actually used?
end | ||
return xs[tid] | ||
end | ||
@inline (once::OncePerThread)() = once[Threads.threadid()] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would put this above since it's the primary API.
end | ||
OncePerThread{T}(initializer::F) where {T, F} = OncePerThread{T,F}(initializer) | ||
OncePerThread(initializer) = OncePerThread{Base.promote_op(initializer), typeof(initializer)}(initializer) | ||
@inline function getindex(once::OncePerThread, tid::Integer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling, this getindex might be a bit too cute? Could just be an optional argument to the call method, i.e.:
function (once::OncePerThread)(tid::Integer = Threads.threadid())
``` | ||
""" | ||
mutable struct OncePerProcess{T, F} | ||
x::Union{Nothing,T} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
x::Union{Nothing,T} | |
value::Union{Nothing,T} |
Calling this value
would be clearer.
return [Libc.rand()] | ||
end; | ||
|
||
julia> procstate = global_state(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Showing the value here would be helpful. Will cause problems with the doctest, but generating random values is kind of a bad example anyway. Maybe we can brainstorm a more meaningful example...
Would have been helpful (to me at least), to make clear that global_state
returns a OncePerProcess{T}
object and calling that object returns a T
object.
This adds 3 new types, to conveniently express 3 common concurrent code patterns:
PerProcess
: an action that must be taken once per processPerThread
: an action that must be taken once per thread idPerTask
: an action that must be take once per task objectThe PerProcess object should replace
__init__
or similar hand rolled implementations of this.The PerThread object should replace code that used to use
nthreads()
to implement a much less correct version of this (though this is not recommended in most new code, some foreign libraries may need this to interact well with C).The PerTask object is simply a thin wrapper over
task_local_storage()
.For example, the jldoctest demonstrates how to lazily allocate a shared constant vector unique to the process: